Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add command jj annotate/blame #4176

Merged
merged 1 commit into from
Oct 15, 2024
Merged

Add command jj annotate/blame #4176

merged 1 commit into from
Oct 15, 2024

Conversation

allonsy
Copy link
Contributor

@allonsy allonsy commented Jul 29, 2024

This change adds a new annotate command (in git this is blame).
In order to annotate a file, call: jj annotate <FILE_PATH>.
In response, jj will traverse the commit graph, trying to find the source of the change for each line.
The output is something similar to this:

>>> jj annotate file.txt
tqqpzkzp bd94545f Alec Snyder 2024-07-29 18:39:13 1: this is an initial line
nqvnwumz b3a07c79 Alec Snyder 2024-07-29 18:39:35 2: this line is commit 1
wlpkktol 5f7de224 Alec Snyder 2024-07-29 18:40:01 3: this line is commit 2

I've also added a new module to jj_lib which exposes a function get_annotation_for_file which
returns a line by line evaluation of the the given file in a more general interface.

A few caveats

  • directories aren't supported
  • conflicted files aren't supported as I'm not exactly sure what to print out in these cases.

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (cli/src/config-schema.json)
  • I have added tests to cover my changes

@allonsy
Copy link
Contributor Author

allonsy commented Jul 29, 2024

As far as the test cases I've thrown at it, it matches with git blame but if anyone finds a counterexample, I'm happy to take a look and see where my bugs are

Copy link
Contributor

@PhilipMetzger PhilipMetzger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a initial comment, it should also mention that this starts work on #2988.

CHANGELOG.md Show resolved Hide resolved
lib/src/annotate.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@PhilipMetzger PhilipMetzger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a second review.

lib/src/annotate.rs Outdated Show resolved Hide resolved
lib/src/annotate.rs Outdated Show resolved Hide resolved
lib/src/annotate.rs Outdated Show resolved Hide resolved
lib/src/annotate.rs Outdated Show resolved Hide resolved
lib/src/annotate.rs Outdated Show resolved Hide resolved
cli/tests/test_annotate_command.rs Outdated Show resolved Hide resolved
@ilyagr
Copy link
Contributor

ilyagr commented Jul 29, 2024

conflicted files aren't supported as I'm not exactly sure what to print out in these cases.

I think that it should figure out which side of the conflict comes from which parent of the commit. Any sides that don't come from a parent (e.g. after a rebase) can be attributed as being added to the commit itself.

Infrastructure related to #4062 would be helpful here.

Of course, none of this is necessary for a first version of annotate functionality.

lib/src/annotate.rs Outdated Show resolved Hide resolved
lib/src/annotate.rs Outdated Show resolved Hide resolved
lib/src/annotate.rs Outdated Show resolved Hide resolved
lib/src/annotate.rs Outdated Show resolved Hide resolved
lib/src/annotate.rs Outdated Show resolved Hide resolved
@martinvonz
Copy link
Member

Thanks for working on this! I have not looked at the implementation yet, but I'll just quickly note that I think a TUI for the functionality would be really useful. I quite often end up trying to find where the previous version of a line came from, and quite often I end up repeating that process several times. GitHub has a "Blame prior to change abc123..." button for each line (or span of lines) for that. It's probably not a big deal to redo the calculation when the user asks for that option, but ideally we could reuse some of the information we already calculated.

@allonsy
Copy link
Contributor Author

allonsy commented Jul 30, 2024

Thanks for working on this! I have not looked at the implementation yet, but I'll just quickly note that I think a TUI for the functionality would be really useful. I quite often end up trying to find where the previous version of a line came from, and quite often I end up repeating that process several times. GitHub has a "Blame prior to change abc123..." button for each line (or span of lines) for that. It's probably not a big deal to redo the calculation when the user asks for that option, but ideally we could reuse some of the information we already calculated.

That's a nice idea. I'm hoping to merge this in and maybe in a future PR I can implement a TUI? Unless you think implementing a TUI would radically change things enough to not make it worth it to merge this in now?

@martinvonz
Copy link
Member

I'm hoping to merge this in and maybe in a future PR I can implement a TUI?

Sounds good. (And I didn't mean to ask you to create a TUI, but sounds great if you're interested in doing that.)

Unless you think implementing a TUI would radically change things enough to not make it worth it to merge this in now?

Nope. I just mentioned it in case it would help any future work in this area.

@allonsy
Copy link
Contributor Author

allonsy commented Jul 30, 2024

conflicted files aren't supported as I'm not exactly sure what to print out in these cases.

I think that it should figure out which side of the conflict comes from which parent of the commit. Any sides that don't come from a parent (e.g. after a rebase) can be attributed as being added to the commit itself.

Infrastructure related to #4062 would be helpful here.

Of course, none of this is necessary for a first version of annotate functionality.

I agree with this for conflicts, I initially tried this but gave up for the first iteration as currently the conflict writers (lib/src/conflict.rs) doesn't have the commit information. So I'd have to forward that through the callers. Definitely doable but a little out of scope for this PR I think

@allonsy allonsy force-pushed the annotate branch 3 times, most recently from 8090b5f to f62815a Compare July 30, 2024 18:40
cli/src/commands/annotate.rs Outdated Show resolved Hide resolved
@allonsy allonsy force-pushed the annotate branch 2 times, most recently from 684840d to ae48961 Compare August 6, 2024 16:55
Copy link
Member

@martinvonz martinvonz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not done reviewing yet, but here are my comments so far

CHANGELOG.md Outdated Show resolved Hide resolved
cli/src/commands/annotate.rs Outdated Show resolved Hide resolved
cli/src/commands/annotate.rs Outdated Show resolved Hide resolved
cli/src/commands/annotate.rs Outdated Show resolved Hide resolved
cli/src/commands/annotate.rs Outdated Show resolved Hide resolved
cli/src/commands/annotate.rs Outdated Show resolved Hide resolved
cli/src/commands/annotate.rs Outdated Show resolved Hide resolved
lib/src/annotate.rs Outdated Show resolved Hide resolved
lib/src/annotate.rs Outdated Show resolved Hide resolved
lib/src/annotate.rs Outdated Show resolved Hide resolved
lib/src/annotate.rs Outdated Show resolved Hide resolved
lib/src/annotate.rs Outdated Show resolved Hide resolved
lib/src/annotate.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@PhilipMetzger PhilipMetzger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG to me after this round, but you should wait for a final approval from either Martin or Yuya.

CHANGELOG.md Show resolved Hide resolved
cli/src/commands/mod.rs Outdated Show resolved Hide resolved
lib/src/annotate.rs Outdated Show resolved Hide resolved
lib/src/annotate.rs Outdated Show resolved Hide resolved
lib/src/annotate.rs Outdated Show resolved Hide resolved
lib/src/annotate.rs Outdated Show resolved Hide resolved
lib/src/annotate.rs Outdated Show resolved Hide resolved
lib/src/annotate.rs Outdated Show resolved Hide resolved
lib/src/annotate.rs Show resolved Hide resolved
lib/src/annotate.rs Outdated Show resolved Hide resolved
@allonsy allonsy force-pushed the annotate branch 4 times, most recently from b446272 to 4c5831a Compare August 7, 2024 17:15
CHANGELOG.md Show resolved Hide resolved
cli/src/commands/annotate.rs Outdated Show resolved Hide resolved
cli/src/commands/annotate.rs Outdated Show resolved Hide resolved
cli/src/commands/annotate.rs Outdated Show resolved Hide resolved
cli/src/commands/mod.rs Outdated Show resolved Hide resolved
lib/src/annotate.rs Outdated Show resolved Hide resolved
lib/src/annotate.rs Outdated Show resolved Hide resolved
lib/src/annotate.rs Outdated Show resolved Hide resolved
lib/src/annotate.rs Outdated Show resolved Hide resolved
lib/src/annotate.rs Outdated Show resolved Hide resolved
@allonsy allonsy force-pushed the annotate branch 2 times, most recently from 176b802 to 6eaff71 Compare August 13, 2024 18:11
lib/src/annotate.rs Show resolved Hide resolved
lib/src/annotate.rs Outdated Show resolved Hide resolved
lib/src/annotate.rs Outdated Show resolved Hide resolved
lib/src/annotate.rs Outdated Show resolved Hide resolved
@PhilipMetzger
Copy link
Contributor

What are the remaining blockers here? As I'd really like to finally have a annotate command, even if it is not perfect yet.

@yuja
Copy link
Contributor

yuja commented Oct 11, 2024

What are the remaining blockers here? As I'd really like to finally have a annotate command, even if it is not perfect yet.

This and some related changes?
#4176 (comment)

I can rewrite the implementation as a follow up if he's okay.

@allonsy
Copy link
Contributor Author

allonsy commented Oct 11, 2024 via email

@allonsy
Copy link
Contributor Author

allonsy commented Oct 13, 2024

I think the last big issue remaining is command name.
Its between raw jj annotate or jj file annotate I think file annotate makes the most sense at this point right?
@yuja
@PhilipMetzger

@yuja
Copy link
Contributor

yuja commented Oct 14, 2024

I think the last big issue remaining is command name. Its between raw jj annotate or jj file annotate I think file annotate makes the most sense at this point right?

Yes, I would go with file annotate (and add annotate -> file annotate alias if we find it's the common requirement.)

Copy link
Contributor

@yuja yuja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

CHANGELOG.md Outdated Show resolved Hide resolved
cli/src/commands/annotate.rs Outdated Show resolved Hide resolved
lib/src/annotate.rs Outdated Show resolved Hide resolved
lib/src/annotate.rs Show resolved Hide resolved
lib/src/annotate.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@yuja yuja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

CHANGELOG.md Outdated Show resolved Hide resolved
lib/src/annotate.rs Outdated Show resolved Hide resolved
lib/src/annotate.rs Outdated Show resolved Hide resolved
lib/src/annotate.rs Outdated Show resolved Hide resolved
lib/src/annotate.rs Outdated Show resolved Hide resolved
lib/src/annotate.rs Outdated Show resolved Hide resolved
lib/src/annotate.rs Outdated Show resolved Hide resolved
@allonsy allonsy force-pushed the annotate branch 3 times, most recently from e9bbedf to 3fea76c Compare October 15, 2024 09:38
A new module is added to jj_lib which exposes a function
get_annotation_for_file. This annotates the given file line by line with
commit information according to the commit that made the most recent
change to the line.
Similarly, a new command is added to the CLI called `jj file annotate` which
accepts a file path. It then prints out line by line the commit
information for the line and the line itself. Specific commit
information can be configured via the templates.annotate_commit_summary
config variable
@allonsy
Copy link
Contributor Author

allonsy commented Oct 15, 2024

@yuja addressed the last round of quick fixes. Anything more before merging?

@yuja
Copy link
Contributor

yuja commented Oct 15, 2024

Anything more before merging?

No. Thanks for updating it quickly.

@allonsy allonsy merged commit 470275b into jj-vcs:main Oct 15, 2024
18 checks passed
@allonsy allonsy deleted the annotate branch October 15, 2024 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants